feat(telemetry-utils): deprecate LogLevel.default/error and tag unspecified events as essential#27207
feat(telemetry-utils): deprecate LogLevel.default/error and tag unspecified events as essential#27207MarioJGMsoft wants to merge 29 commits intomicrosoft:mainfrom
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (102 lines, 13 files), I've queued these reviewers:
Toggle the reviewer checkboxes above to adjust, then tick the box below to start:
|
There was a problem hiding this comment.
Pull request overview
This PR updates Fluid’s telemetry log-level semantics by deprecating legacy LogLevel members and changing how ChildLogger treats events with no explicit logLevel, to better distinguish “essential” telemetry.
Changes:
- Deprecates
LogLevel.defaultandLogLevel.error, and migrates in-repo usages toLogLevel.info/LogLevel.essential. - Changes
ChildLoggerto treat events with no explicitlogLevelasessential, including forwardingessentialto the base logger. - Updates related types/docs, tests, API reports, and adds changesets documenting the behavior change.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/utils/telemetry-utils/src/test/multiSinkLogger.spec.ts | Updates tests to use info/essential instead of deprecated default/error. |
| packages/utils/telemetry-utils/src/test/childLogger.spec.ts | Updates log-level constants in tests; should be extended to cover the new “unspecified logLevel ⇒ essential” behavior. |
| packages/utils/telemetry-utils/src/telemetryTypes.ts | Updates logLevel unions to use info; updates TSDoc (currently inconsistent with new runtime defaults). |
| packages/utils/telemetry-utils/src/mockLogger.ts | Updates MockLogger default min level / filtering fallback from default to info. |
| packages/utils/telemetry-utils/src/logger.ts | Removes parameter defaults, changes ChildLogger fallback/forwarding to essential, updates MultiSink defaults, and adds PerformanceEvent docs (currently inaccurate). |
| packages/utils/telemetry-utils/api-report/telemetry-utils.legacy.beta.api.md | API report update reflecting new logLevel unions. |
| packages/loader/container-loader/src/test/container.spec.ts | Import ordering/type-only import adjustment. |
| packages/common/core-interfaces/src/logger.ts | Adds @deprecated tags for LogLevelConst.default/error and updates base logger docs to default to info. |
| packages/common/core-interfaces/api-report/core-interfaces.public.api.md | API report reflects deprecated members. |
| packages/common/core-interfaces/api-report/core-interfaces.legacy.public.api.md | API report reflects deprecated members. |
| packages/common/core-interfaces/api-report/core-interfaces.legacy.beta.api.md | API report reflects deprecated members. |
| packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md | API report reflects deprecated members. |
| packages/common/core-interfaces/api-report/core-interfaces.beta.api.md | API report reflects deprecated members. |
| .changeset/soft-doors-trade.md | Changeset documenting the ChildLogger unspecified-logLevel behavior change and downstream impact. |
| .changeset/salty-colts-appear.md | Changeset documenting the LogLevel.default/error deprecations and migration guidance. |
| * Log a telemetry event, if it meets the appropriate log-level threshold (see {@link ITelemetryBaseLogger.minLogLevel}). | ||
| * @param event - The event to log. | ||
| * @param logLevel - The log level of the event. Default: {@link LogLevelConst.default | LogLevel.default}. | ||
| * @param logLevel - The log level of the event. Default: {@link LogLevelConst.info | LogLevel.info}. |
There was a problem hiding this comment.
Isn't the default supposed to be essential? Maybe it's clearer if we say it like "suggested interpretation of undefined: LogLevel.essential"? Since this is an interface so claiming a "default value" is kind of in vain, it could be implemented any way.
Removes 11 zero-byte files added by mistake in 2c13181. These are bind-mounted from the dev container host (shell rc files, IDE config, MCP config) and don't belong in the repo.
jason-ha
left a comment
There was a problem hiding this comment.
A few docs and test suggestions. Thanks!
|
Please also remember to update your PR description |
jason-ha
left a comment
There was a problem hiding this comment.
I'm going to approve - you'll need docs sign-off anyway
|
|
||
| - For an **event's `logLevel`** (e.g. the `logLevel` argument to `sendTelemetryEvent` / `sendPerformanceEvent` / `ITelemetryBaseLogger.send`), the recommendation is `LogLevel.essential`. | ||
| - For a logger's **`minLogLevel`** (the threshold that filters events), `LogLevel.info` is the recommendation. | ||
| - For an **event's default `logLevel`** (e.g. the `logLevel` argument to `sendTelemetryEvent` / `sendPerformanceEvent` / `ITelemetryBaseLogger.send`), the recommendation is `LogLevel.essential`. |
There was a problem hiding this comment.
This is a little funny because we don't want anyone implementing sendTelemetryEvent / sendPerformanceEvent. Just ITelemetryBaseLogger.send will do.
| - For an **event's default `logLevel`** (e.g. the `logLevel` argument to `sendTelemetryEvent` / `sendPerformanceEvent` / `ITelemetryBaseLogger.send`), the recommendation is `LogLevel.essential`. | ||
| - For a logger's **default `minLogLevel`** (the threshold that filters events), `LogLevel.info` is the recommendation. | ||
|
|
||
| The replacement for `LogLevel.error` should always be essential. |
There was a problem hiding this comment.
"essential" -> "LogLevel.essential" so the replacement is symmetric with the source.
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
jzaffiro
left a comment
There was a problem hiding this comment.
Minor nit, but approved for docs!
| "__section": deprecation | ||
| --- | ||
|
|
||
| Deprecated LogLevel.default and LogLevel.error |
There was a problem hiding this comment.
| Deprecated LogLevel.default and LogLevel.error | |
| Deprecate LogLevel.default and LogLevel.error |
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
| return; | ||
| } | ||
| this.baseLogger.send(this.prepareEvent(event), logLevel); | ||
| this.baseLogger.send(this.prepareEvent(event), logLevel ?? LogLevel.essential); |
There was a problem hiding this comment.
What happened to making logLevel required on send for the sub-classes? Now we have two places in the same flow filling in defaults
There was a problem hiding this comment.
It couldn't be made there might still be chances that ChildLogger and Multisink receive undefined from old containerRuntime (At least that's what I understood). @jason-ha to confirm
| * @param error - Optional error object to log. | ||
| * @param logLevel - Optional level of the log. Default: {@link @fluidframework/core-interfaces#LogLevel.essential}. | ||
| * @param logLevel - Optional level of the log. If undefined, the logLevel should be treated as {@link @fluidframework/core-interfaces#LogLevel.essential}. | ||
| * If the event's category is `error`, the logLevel will be upgraded to {@link @fluidframework/core-interfaces#LogLevel.essential}. |
There was a problem hiding this comment.
super nit:
| * If the event's category is `error`, the logLevel will be upgraded to {@link @fluidframework/core-interfaces#LogLevel.essential}. | |
| * If the event's category is `error`, the logLevel should be upgraded to {@link @fluidframework/core-interfaces#LogLevel.essential}. |
There was a problem hiding this comment.
So this is referring to the flow in sendTelemetryEvent and sendPerformanceEvent , where they do this: event.category === "error" ? LogLevel.essential : (logLevel ?? LogLevel.essential), so if it's an error, we will upgrade it to essential no matter what.
| logLevel?: typeof LogLevel.verbose | typeof LogLevel.info, | ||
| ): void; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Add a note here that it's going to use logLevel essential?
There was a problem hiding this comment.
If you're referring to sendTelemetryEvent, that note already exists on line 188. If you're referring to sendErrorEvent, since the change itself doesn't affect the interface, I don't think it's necessary to comment that there.
Description
Tracks issue #26969 (planned removal in v3.0).
This PR has two parts:
1. Deprecate
LogLevel.defaultandLogLevel.errorAdds
@deprecatedtags onLogLevel.defaultandLogLevel.errorand migratesall in-repo callsites:
Adds
@deprecatedtags onLogLevel.defaultandLogLevel.errorin@fluidframework/core-interfacesand migrates all in-repo callsites:LogLevel.default→ context-dependent (see changeset / Remove deprecated LogLevel.default and LogLevel.error #26969):logLevelargument:LogLevel.essentialminLogLevelthreshold:LogLevel.infoLogLevel.error→LogLevel.essentialAffected files include
telemetry-utils(logger.ts,mockLogger.ts,telemetryTypes.ts, and the related tests) and the corresponding API reports.2. Treat events with no explicit
logLevelasessentialinTelemetryLoggerTwo related changes in
packages/utils/telemetry-utils/src/logger.ts:ChildLogger.shouldFilterOutEventnow falls back toLogLevel.essential(30) when an event arrives without alogLevel(previouslyLogLevel.default=20).ChildLogger.sendnow forwardsLogLevel.essentialto the base logger when nologLevelwas supplied, instead of forwardingundefined.Additionally, the parameter defaults on
TelemetryLogger.sendTelemetryEventandsendPerformanceEventwere dropped (they were= LogLevel.default). Callers that don't pass alogLevelnow propagateundefinedthrough the chain, whereChildLoggerapplies the newessentialfallback above.3.
createSampledLoggerandMultiSinkLogger.sendnow forwarderrorandlogLevelTwo pre-existing bugs surfaced while wiring up the
essentialfallback above:createSampledLogger(packages/utils/telemetry-utils/src/utils.ts) silently droppederrorandlogLevelarguments before delegating to the wrapped logger — the wrapper's method signatures didn't even accept those arguments. The wrapper now accepts and forwards them:sendacceptslogLevel?sendTelemetryEventacceptserror?andlogLevel?(verbose | info)sendErrorEventacceptserror?sendPerformanceEventacceptserror?andlogLevel?(verbose | info)MultiSinkLogger.send(packages/utils/telemetry-utils/src/logger.ts) had the same bug —logLevelwas dropped before fanning out to sinks. It now acceptslogLevel?and forwards it to every registered sink (defaulting toLogLevel.essentialwhen omitted, consistent with the part 2 behavior).New test coverage:
telemetryLogger.spec.ts— newdescribe("logLevel forwarding")coveringsendTelemetryEvent,sendPerformanceEvent, andsendErrorEvent: default fallback toessential, explicitinfoforwarding, andcategory: "error"upgrading toessentialeven wheninfowas requested. (These tests live on the parent class becausesendTelemetryEvent/sendPerformanceEventare implemented there, not onChildLogger.)childLogger.spec.ts—describe("logLevel forwarding")verifiesChildLogger.sendpropagates the level through to the underlying sink (essentialdefault, explicitinfo, andverboseonce the sink opts in viaminLogLevel).multiSinkLogger.spec.ts—describe("logLevel propagation")verifies theessentialfallback, explicit-level forwarding for bothsendTelemetryEventandsendPerformanceEvent, and includes a regression test for theMultiSinkLogger.senddrop bug.utils.spec.ts—describe("logLevel forwarding")verifies the sampled logger forwardslogLevelthroughsend,sendTelemetryEvent, andsendPerformanceEvent, and propagatesundefinedwhen the caller omits it.Misc
typeimport ordering incontainer.spec.ts.Behavior change for downstream consumers
Although no public API is removed in this PR, downstream consumers that filter telemetry by numeric level should be aware:
Internal Fluid events that were previously emitted to the base logger with
logLevel = undefined(effectively treated as20) will now arrive tagged as30(essential). If a host application starts droppinglogLevel = 20eventsas "non-essential", it must ensure all running Fluid versions include this
change — older Fluid versions still send those same critical events at level
20, and dropping them would lose telemetry.Breaking Changes
No public API surface is removed. See #26969 for the planned v3.0 removal of
LogLevel.defaultandLogLevel.error. See the "Behavior change for downstream consumers" section above for a runtime behavior note.Reviewer Guidance
The review process is outlined on this wiki page.
Specific asks:
LogLevel.essentialfallback inChildLogger.shouldFilterOutEventandChildLogger.sendis the intended semantic (vs. leavingundefinedto flow to the base logger).= LogLevel.defaultparameter defaults onsendTelemetryEvent/sendPerformanceEventis acceptable for consumers who may have relied on the implicit default.createSampledLoggerandMultiSinkLogger.sendargument forwarding fix is acceptable as part of this PR (it changes observable behavior for hosts wrapping with these helpers).